cli: scaffold cobra-based devcontainer CLI#79
Merged
Conversation
Add cmd/devcontainer, a thin CLI on top of the engine. The CLI is meant for humans at a terminal — logs to stderr, results to stdout, non-zero exit on failure. Tools wanting programmatic access import the library directly, so no outcome-envelope JSON shim. Commands wired: up Engine.Up + streamed event progress exec Attach + Exec with raw-mode TTY + SIGWINCH down / stop Engine.Down (with/without remove) read-configuration Resolve, JSON to stdout run-user-commands Iterate Engine.RunLifecycle across phases Global flags: --workspace-folder, --config, --runtime (docker | applecontainer, default docker), --log-level. Subcommand-specific flags mirror the upstream @devcontainers/cli names where they map cleanly (--remove-existing-container, --run-initialize-command, --run-secrets-command, --remove-volumes). Follow-ups tracked in #74-#78 (log-level plumbing, exec --env, --recreate alias, ResolvedConfig json tags, Engine.Build). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughA new CLI package adds a ChangesDevContainer CLI Implementation
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/devcontainer/exec.go`:
- Around line 63-73: The struct literal for execOpts uses fields that don’t
exist on devcontainer.ExecOptions and the code refers to a non-existent
runtime.TtySize; update cmd/devcontainer/exec.go to match the real API by
removing InitialTtySize and ResizeCh from the devcontainer.ExecOptions literal
(leave Cmd/Env/User/WorkingDir/Tty/Stdin/Stdout/Stderr only) and remove any uses
of runtime.TtySize/resize-channel plumbing in setupTty and the surrounding logic
so the implementation relies only on the Tty boolean and standard streams;
ensure execOpts is passed to eng.Exec and setupTty only toggles terminal mode
when Tty is true to restore compilation against the actual devcontainer/runtime
ExecOptions API.
In `@cmd/devcontainer/root.go`:
- Line 38: The CLI exposes a no-op flag created with pf.StringVar(&f.logLevel,
"log-level", "info", ...) but f.logLevel is never applied; either
remove/deprecate the flag or wire it into your logger setup: read f.logLevel in
the command initialization (where you call your logger bootstrap, e.g.,
setupLogger/initLogger/CreateLogger) and map allowed values
("info"|"debug"|"trace") to the logger's level API, validating unknown values
and returning a user-facing error; if you choose deprecation, mark the flag
hidden or remove the pf.StringVar call and document the removal.
- Around line 80-85: The applecontainer branch returns rt (created by
applecontainer.New) as runtime.Runtime but the stub applecontainer.Runtime on
non-darwin builds lacks BuildImage causing compile errors; fix by wrapping or
adapting the returned rt to satisfy runtime.Runtime: after rt, check/assert
whether rt implements the BuildImage method (or the full runtime.Runtime
interface) and if not return a clear error, or create a small adapter type that
embeds applecontainer.Runtime and implements BuildImage (returning an
appropriate unsupported error) so the returned value always implements
runtime.Runtime; reference applecontainer.New, applecontainer.Options{},
applecontainer.Runtime, and the BuildImage method when locating where to add the
check/adapter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7b94564-80fc-4ea4-bd86-4b1693d8d8aa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
cmd/devcontainer/down.gocmd/devcontainer/events.gocmd/devcontainer/exec.gocmd/devcontainer/main.gocmd/devcontainer/read_configuration.gocmd/devcontainer/root.gocmd/devcontainer/run_user_commands.gocmd/devcontainer/up.gogo.mod
CI was red because the first commit reached for runtime API surface that lives in an in-progress local branch, not main: - runtime.TtySize / ExecOptions.InitialTtySize / ExecOptions.ResizeCh aren't on main yet, so exec.go failed to compile in CI. Strip the SIGWINCH forwarding for now; TTY raw mode still works. Re-add window resize once the runtime change lands. - runtime/applecontainer's non-darwin stub doesn't implement runtime.Runtime, so importing it unconditionally broke the linux build. Move applecontainer wiring into a build-tagged file (darwin/arm64 only); other platforms return a clear error from --runtime=applecontainer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
golangci-lint surfaced two real issues: - 18 unchecked fmt.Fprintf returns to stderr (errcheck). Centralise the swallow in two tiny helpers (stderrf / outf) so the silenced error is named once, not scattered around. Writing to stderr has no actionable recovery; the explicit ignore is more honest than a blanket exclusion in .golangci.yml. - exitCodeFor was unused (main.go uses errors.As directly). Removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
cmd/devcontainer, a thin Cobra-based CLI on top of the engine. The CLI is for humans at a terminal — logs to stderr, results to stdout, non-zero exit on failure. Programmatic consumers import the library directly, so there's no outcome-envelope JSON shim (the whole point of this project being a Go library, not another shellout target).Commands wired in this PR:
upEngine.Up+ streamed event progressexecAttach+Execwith raw-mode TTY and SIGWINCH forwardingdown/stopEngine.Down(with / withoutRemove)read-configurationResolve→ JSON to stdoutrun-user-commandsEngine.RunLifecycleacross phasesGlobal flags:
--workspace-folder,--config,--runtime=docker|applecontainer(defaultdocker),--log-level. Subcommand flags mirror the upstream@devcontainers/clinames where they map cleanly (--remove-existing-container,--run-initialize-command,--run-secrets-command,--remove-volumes).Smoke-tested locally against a minimal image-only devcontainer: full lifecycle (
up→exec→read-configuration→run-user-commands→stop→down) green, exit codes propagate (e.g. in-containerexit 7→ CLI exit 7).Follow-ups (already filed)
--log-levelinto event filter (today it's a no-op)exec#75 —exec --env KEY=VALup --recreateas an alias for--remove-existing-container#76 —up --recreatealiasResolvedConfigfor stableread-configurationoutputbuild) #78 —Engine.Build(prerequisite for a CLIbuildsubcommand)Why draft
Holding for review on:
devcontainer— collides with upstream onPATH; happy to rename if that's a problem)--log-levelshould be dropped from this PR until cli: plumb --log-level into event filter (or drop the flag) #74 lands, rather than shipping a no-op flag--remove-existing-container) in favor of library-style names (--recreate) now, rather than aliasing laterTest plan
go build ./cmd/devcontainerdevcontainer upagainst a fixture workspacedevcontainer exec -- sh -c 'echo hi'returns 0devcontainer exec -- sh -c 'exit 7'exits 7devcontainer read-configuration | jq .parsesdevcontainer run-user-commandsruns all phasesdevcontainer stopleaves container stopped; subsequentuprestarts itdevcontainer downremoves the container🤖 Generated with Claude Code
Summary by CodeRabbit